Support history_file configuration per dsn, and --history option. #1216
Conversation
pgcli/main.py
Outdated
| @@ -1194,6 +1194,9 @@ def echo_via_pager(self, text, color=None): | |||
| @click.option( | |||
| "--warn/--no-warn", default=None, help="Warn before running a destructive query." | |||
| ) | |||
| @click.option( | |||
| "--history", default=None, help="Specify history file location." | |||
There was a problem hiding this comment.
Perhaps histfile, so it matches the name used by psql?
There was a problem hiding this comment.
Excellent! I knew I met --histfile somewhere but I forgot it's psql, I just haven't use psql for a long time. :)
| @@ -187,6 +188,11 @@ output.null = "#808080" | |||
| [alias_dsn] | |||
| # example_dsn = postgresql://[user[:password]@][netloc][:port][/dbname] | |||
|
|
|||
| # Specify sperate history file for alias_dsn. | |||
| # If not set, will use history_file settings. | |||
| [dsn_history_file] | |||
There was a problem hiding this comment.
We can support mapping of history file to not only DSN aliases, but database names as well. That way, if you have mydb1 hosted on different servers, you could just configure:
[db_history_file]
mydb1 = ~/config/pgcli/history_mydb1
this could be useful to users that do not use DSN aliases.
There will be ambiguity if we do that: we'd have to figure out if mydb1 is a DSN alias or a database name. We can look for matching DSN alias first, if user provided --dsn when running the cli, and if there's no alias found, assume this is database name.
There was a problem hiding this comment.
Yes, I like the idea that mapping of the history file for the database. I think it covers most of the use cases. Maybe consider alias_dsn is kind of overthinking and over-designed, I think we add too mush to configuration, and cloud leads to misunderstanding. I don't think users want to share history between databases. Even if they want to do that, they can use --histfile option.
Let's see, we only add a flag sperate_history_on_database_name = False, and add --histfile option. (Also, if user use \c[onnect] database_name, we should switch history file as well.
If the user does want to share history between database, they can:
- Use
--histfile, maybe use shell's alias together. - Save the most used query to
named query.
What do you think?
There was a problem hiding this comment.
I think that:
- separating history per database does not need a config flag. Once that feature is in, we should do it by default. When you're connected to a specific database, queries from other databases are not very useful / relevant, and would only clutter history.
- user can use
--histfileto override the default history-file-per-database. - mapping to DSN vs database name can be useful. I had a use case with
prodandbetadatabase tables namedtable1_prod,table2_prodetc. Table structure was the same, but table names were different, so you could not use exactly the same query on those databases. This may be an edge case.
There was a problem hiding this comment.
I am convinced, I will only add a section named dsn_history_file section(maybe we cloud come up with a better name), and left other configs untouched.
So the process of choosing a history file will look like this:
Do I understand it correctly?
There was a problem hiding this comment.
Yes, this looks exactly right.
I added two args for PGCli init, because I think we need to figure out which history file to use on `__init__`. And we will need `histfile` `alias_dsn` for that. PS: I don't think change PGCli's property after initializing is good.
laixintao
force-pushed
the
feature/history-config
branch
from
639158e
to
031325a
Compare
Codecov Report
@@ Coverage Diff @@
## master #1216 +/- ##
==========================================
- Coverage 83.78% 83.76% -0.02%
==========================================
Files 21 21
Lines 2559 2563 +4
==========================================
+ Hits 2144 2147 +3
- Misses 415 416 +1
Continue to review full report at Codecov.
|
|
I like this feature and would love this PR to come through. @laixintao are you still working on this. Is there something I can do to help? |
|
Yes still working on it, but I just changed my job and got married last
month so this PR was delayed.
I will get back to this recently, sorry.
…On Wed, Jan 13, 2021 at 1:27 PM Guru Devanla ***@***.***> wrote:
I like this feature and would love this PR to come through. @laixintao
<https://github.com/laixintao> are you still working on this. Is there
something I can do to help?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1216 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACJ2JIYLC32ZEQUNG5IDP5DSZUVKRANCNFSM4SR22KUQ>
.
|
|
I am so sorry, totally forgot this PR. I will get back to this |
|
@laixintao i'd like to help finish this one, do you mind if i try to pick it up? |
sure, thanks! (shame on me 🥺) |
Description
This feature enables the user to config history file location for specific dsn.
close: #535
TODOs
--hisfilesupportChecklist
changelog.rst.AUTHORSfile (or it's already there).pip install pre-commit && pre-commit install), and ranblackon my code.